fix(store): surface FlushFileBuffers failure in journal SYNC mode#335
Merged
singaraiona merged 1 commit intoJul 18, 2026
Merged
Conversation
In RAY_JOURNAL_SYNC mode ray_journal_write_bytes checks fsync's return on POSIX and fails the write with RAY_ERR_IO, but the Windows branch ignored FlushFileBuffers' return. A failed flush there was silently swallowed, so SYNC mode reported success while the data may not have reached disk — dropping the durability guarantee the mode exists to provide. Check FlushFileBuffers (0 = failure) and return RAY_ERR_IO, mirroring the POSIX path. Windows-only branch (not built on the Linux/macOS CI matrix), so it is verified by inspection against the adjacent fsync check; the failure path is not unit-testable, like the existing POSIX one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
In
RAY_JOURNAL_SYNCmode,ray_journal_write_bytes(src/store/journal.c) ismeant to guarantee each write reaches disk before returning — that is the whole
point of SYNC mode (crash recovery). The POSIX branch enforces this by checking
fsync's return and failing withRAY_ERR_IO; the Windows branch calledFlushFileBuffersbut ignored its return. A failed flush was silentlyswallowed, so the function returned
RAY_OKwhile the data may not have reacheddisk — dropping the durability guarantee the mode exists to provide.
The fix checks
FlushFileBuffers(returns 0 /FALSEon failure) and returnsRAY_ERR_IO, mirroring the adjacentfsynccheck.Verification & scope
This branch is Windows-only. The project's CI matrix is
ubuntu-latest+macos-latest(no Windows leg), so the changed line is not compiled by CI andnot reproducible locally on macOS. It is a one-line symmetry fix that uses only
symbols already present in the original Windows branch, verified by inspection
against the
fsyncpath two lines above.No test is added: the flush-failure path is not unit-testable (a successful
FlushFileBuffers/fsynccan't be made to fail on demand), which is also whythe equivalent POSIX
fsync-failure path is untested. The existing SYNC-modewrite test (
test_journal.c, case 4f) still covers the success path. The fullmacOS debug (ASan + UBSan) suite builds clean and passes:
3631 of 3632 passed (1 skipped, 0 failed).Checklist
dev(notmaster)fix:)makebuilds cleanly (no new warnings)make testpasses (no unit test added — the Windows-only flush-failurepath is not testable on demand; see Verification & scope)